[rust] honor --browser-version in Selenium Manager Electron driver resolution#17567
Open
gaurav0107 wants to merge 2 commits into
Open
[rust] honor --browser-version in Selenium Manager Electron driver resolution#17567gaurav0107 wants to merge 2 commits into
gaurav0107 wants to merge 2 commits into
Conversation
…solution Fixes SeleniumHQ#17549. ElectronManager::request_driver_version() always read the redirect from electron/electron/releases/latest, discarding any user-supplied --browser-version. Because Electron release tags equal the chromedriver version that ships with that release, when the user pins a concrete browser version we can use it directly and skip the network roundtrip. Stable / unstable / empty browser versions retain the existing /releases/latest fallback. Also gate the metadata cache write on a non-empty major_browser_version, mirroring the chrome.rs guard, so empty-keyed driver entries no longer get persisted.
selenium-ci
added
C-rust
Assert the resolved driver path actually contains the requested browser version. Without this, the test only verified that selenium manager exits successfully, which the buggy code path also did (it just downloaded the wrong version). Using --output json + the existing get_driver_path helper makes the regression assertion explicit.
|
|
Contributor
Review Summary by QodoHonor --browser-version in Selenium Manager Electron driver resolution
WalkthroughsDescription• Honor user-supplied --browser-version in Electron driver resolution • Use concrete browser version directly instead of always fetching /releases/latest • Gate metadata cache writes on non-empty major_browser_version to prevent empty-keyed entries • Add regression test asserting resolved driver path contains requested version Diagramflowchart LR
A["User supplies --browser-version"] --> B{Is concrete version?}
B -->|Yes| C["Use version directly as driver version"]
B -->|No| D["Fetch /releases/latest redirect"]
C --> E["Resolve driver path"]
D --> E
E --> F["Cache metadata if version non-empty"]
File Changes1. rust/src/electron.rs
|
Contributor
Code Review by Qodo
1. Electron treats major as tag
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+128
to
+134
| let browser_version = self.get_browser_version().to_string(); | ||
| let driver_version = if !browser_version.is_empty() | ||
| && !self.is_browser_version_stable() | ||
| && !self.is_browser_version_unstable() | ||
| { | ||
| browser_version | ||
| } else { |
Contributor
There was a problem hiding this comment.
1. Electron treats major as tag 📘 Rule violation ≡ Correctness
ElectronManager::request_driver_version() now treats any non-empty --browser-version that is not stable/unstable as an exact Electron release tag, which incorrectly includes major-only inputs like 36 that are documented as valid. This can break existing CLI usage by producing invalid Electron download URLs (e.g., .../download/v36/...) and causing downloads to fail.
Agent Prompt
## Issue description
`ElectronManager::request_driver_version()` currently returns `--browser-version` verbatim as the Electron `driver_version` for any non-empty input that is not the `stable`/`unstable` channel. This mistakenly treats major-only values (e.g., `36`)—which the CLI documents as valid major-version inputs—as exact Electron release tags, leading to invalid tag/URL construction (e.g., `.../download/v36/...`) and failed downloads.
## Issue Context
The codebase already distinguishes a “specific” version from a major version using the presence of a dot (`.`) (i.e., “specific” means it contains a dot). The CLI help for `--browser-version` explicitly describes it as a major version input and also supports channel names. Electron’s direct-return/short-circuit behavior should therefore only apply when the provided browser version is actually specific (e.g., `36.2.1`), while major-only values should continue to use the existing latest-redirect resolution (e.g., `/releases/latest`) or alternatively produce a deterministic validation error; stable/unstable handling should remain unchanged.
## Fix Focus Areas
- rust/src/electron.rs[103-155]
- rust/src/lib.rs[813-819]
- rust/src/main.rs[65-68]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+128
to
+142
| let browser_version = self.get_browser_version().to_string(); | ||
| let driver_version = if !browser_version.is_empty() | ||
| && !self.is_browser_version_stable() | ||
| && !self.is_browser_version_unstable() | ||
| { | ||
| browser_version | ||
| } else { | ||
| self.assert_online_or_err(OFFLINE_REQUEST_ERR_MSG)?; | ||
| let latest_url = format!( | ||
| "{}{}", | ||
| self.get_driver_mirror_url_or_default(DRIVER_URL), | ||
| LATEST_RELEASE | ||
| ); | ||
| read_redirect_from_link(self.get_http_client(), latest_url, self.get_logger())? | ||
| }; |
Contributor
There was a problem hiding this comment.
2. Unnormalized pinned version 🐞 Bug ≡ Correctness
The pinned --browser-version is used verbatim as driver_version, but get_driver_url always prefixes the driver version with "v", so inputs like "v36.2.1" will produce "vv36.2.1" and break downloads. This is a regression from the redirect-based path that normalizes versions via parse_version().
Agent Prompt
### Issue description
When `--browser-version` is treated as the driver version, it is not normalized. A common user input for GitHub tags is a leading `v` (e.g., `v36.2.1`), but `get_driver_url()` already adds its own `v` prefix, yielding `vv...` and a 404.
### Issue Context
The existing online-redirect resolution path calls `read_redirect_from_link()`, which normalizes the redirect URL via `parse_version()` (stripping non-numeric prefixes such as `v`). The new bypass path skips this normalization.
### Fix Focus Areas
- rust/src/electron.rs[103-176]
- rust/src/downloads.rs[89-99]
- rust/src/files.rs[530-548]
### What to change
- Before returning `browser_version` as `driver_version`, normalize it:
- trim whitespace
- if it starts with `v`/`V`, strip that leading character
- (optional) validate it’s a specific version format used by Electron assets
- Ensure the normalized value is what gets used for URL construction and cache paths.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
119
to
+134
| Ok(driver_version) | ||
| } | ||
| _ => { | ||
| self.assert_online_or_err(OFFLINE_REQUEST_ERR_MSG)?; | ||
|
|
||
| let latest_url = format!( | ||
| "{}{}", | ||
| self.get_driver_mirror_url_or_default(DRIVER_URL), | ||
| LATEST_RELEASE | ||
| ); | ||
| let driver_version = | ||
| read_redirect_from_link(self.get_http_client(), latest_url, self.get_logger())?; | ||
| // Electron releases are tagged by Electron version, and the | ||
| // chromedriver asset shipped in each release matches that tag. | ||
| // When the user pins a concrete browser version (e.g. | ||
| // `--browser-version 36.2.1`), that version is the driver | ||
| // version we want; resolving via `/releases/latest` would | ||
| // discard the user's request and return the latest tag instead. | ||
| let browser_version = self.get_browser_version().to_string(); | ||
| let driver_version = if !browser_version.is_empty() | ||
| && !self.is_browser_version_stable() | ||
| && !self.is_browser_version_unstable() | ||
| { | ||
| browser_version | ||
| } else { |
Contributor
There was a problem hiding this comment.
3. Cache overrides pinned patch 🐞 Bug ≡ Correctness
request_driver_version still checks metadata by major_browser_version before applying the pinned --browser-version logic, so a cached entry for major "36" can override a user request for "36.2.1" and return a different patch driver version. This can silently violate the user’s explicit version pin.
Agent Prompt
### Issue description
Even after this PR, `ElectronManager::request_driver_version()` may return a cached driver version based on `major_browser_version` before it considers the user’s pinned full `--browser-version`. Because the metadata key is only the major component, different patch pins under the same major can conflict.
### Issue Context
- Metadata lookup for driver version keys on `major_browser_version`.
- `get_major_browser_version()` returns only the major component for non-channel inputs.
- The new pinned-version behavior lives only in the metadata-miss branch.
### Fix Focus Areas
- rust/src/electron.rs[103-155]
- rust/src/metadata.rs[123-139]
- rust/src/lib.rs[1365-1374]
### What to change
One of the following (prefer the one consistent with project behavior):
- If `--browser-version` is a specific pinned version, bypass metadata lookup entirely and return the requested (normalized) version.
- Or, key metadata lookups/writes for Electron on the full pinned version (not just the major) when `is_browser_version_specific()` is true, so separate patch pins don’t collide.
Also consider adding a regression test that runs two different `--browser-version 36.x.y` values back-to-back and asserts the second run resolves to the second requested patch.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
ElectronManager::request_driver_version()always read the GitHub redirect fromelectron/electron/releases/latestand used the resolved tag as the driver version. The user-supplied--browser-versionwas only consulted as a metadata cache key (viaget_major_browser_version()); it never affected the actual URL resolution. As a result,get_driver_url()formatted the chromedriver download URL using whatever version came back from/releases/latestrather than the version the user requested.For Electron, the chromedriver shipped inside each release is tagged identically to the Electron release itself (
https://github.com/electron/electron/releases/download/v{VERSION}/chromedriver-v{VERSION}-{platform}.zip), so when the caller already supplies a concrete version, we can use it directly and avoid the network roundtrip altogether.The fix:
_ =>arm ofrequest_driver_version, ifget_browser_version()is a concrete version (non-empty, notstable, not an unstable channel likedev/beta/nightly), return it directly as the driver version./releases/latestredirect path unchanged.major_browser_version(mirrors the existing guard inchrome.rs:338), so we no longer persist empty-keyed driver entries.A regression test (
electron_browser_version_test) was added inrust/tests/electron_tests.rsthat pins--browser-version 36.2.1and asserts the manager succeeds. Locally:The pre-existing
electron_latest_test(which exercises the unchanged latest-fallback path) also still passes.Motivation and Context
Closes #17549.
When Selenium Manager is asked to provision the Electron driver against a specific Electron version, the user expects the chromedriver from that release. Today, every request - regardless of
--browser-version- silently downloads whatever is at/releases/latest, so cross-version testing of Electron apps via SM is effectively broken.Types of changes
Checklist
electron_browser_version_testadded, exercises the new specific-version path;electron_latest_testcontinues to cover the unchanged latest-fallback path.cargo build,cargo fmt --check,cargo test --lib, and bothelectron_*integration tests pass.AI assistance disclosure
Per the CONTRIBUTING.md AI Tool Use section: AI assistance was used to draft the patch and the regression test. I read, reviewed, and ran the changes locally, and I am the responsible author. No
Co-Authored-Bytrailer for AI tooling per project policy.